Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Please remove all of your comments in the code - you do not want to write a comment like
//This is an age variable of type number
const age = 4The code should explain itself
| <!-- Loader --> | ||
| <div class="loader"> | ||
| <div class="loader" id="loader"> | ||
| <div class="spinner"> |
There was a problem hiding this comment.
Why do you need both class and id?
There was a problem hiding this comment.
because I used get element by id in the js.
There was a problem hiding this comment.
Cant you get it via class? I mean you already have a class loader.
| <div class="container"> | ||
| <section class="country-details"></section> | ||
| <section class="country-details"id="country-name"></section> | ||
| </div> |
|
|
||
| // Fetch the JSON data | ||
| fetch('./CountriesData.json') | ||
| .then(response => response.json()) |
There was a problem hiding this comment.
Hardcoded strings should be saved into a constants file
| .then(response => response.json()) | ||
| .then(countries => { | ||
| // Get the country name from the URL parameter | ||
| const urlParams = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
Extract thing into a separate function, huge functions are not readable
| details.forEach(detail => { | ||
| const detailElement = document.createElement('p'); | ||
| detailElement.innerHTML = `<strong>${detail.label}:</strong> ${detail.value}`; | ||
| infoContainer.appendChild(detailElement); |
There was a problem hiding this comment.
Do not use innerHTML it is a dangerous method
| // Nest flag and info into the main container | ||
| countryContainer.appendChild(flagContainer); | ||
| countryContainer.appendChild(infoContainer); | ||
|
|
There was a problem hiding this comment.
Just a tip - if you will need to add a lot of children's save the children into an array and loop the array to remove code duplication
For those 2 lines its ok
| const loader = document.getElementById('loader'); | ||
| if (loader) { | ||
| loader.style.display = 'none'; | ||
| } |
There was a problem hiding this comment.
Do not style your elements with js, use css classes
| <div class="dropdown-header flex flex-jc-sb flex-ai-c"> | ||
| <div class="dropdown-wrapper" id="dropdown-wrapper"> | ||
| <div class="dropdown-header flex flex-jc-sb flex-ai-c" id="dropdown-header"> | ||
| <span>Filter by Region</span> |
There was a problem hiding this comment.
Ids are unique - you should only have 1 of the same id in your page
| @@ -0,0 +1,100 @@ | |||
| fetch('CountriesData.json') | |||
| .then((response) => response.json()) | |||
There was a problem hiding this comment.
Same thing in here about the hardcoded strings into constants
|
|
||
| // global variable | ||
| let allCountries = countries; | ||
|
|
| // view the countries | ||
| function displayCountries(countriesToDisplay) { | ||
| countriesContainer.innerHTML = ''; //cleaning all the current HTML in the div | ||
| countriesToDisplay.forEach((country) => { |
There was a problem hiding this comment.
Same comment in here do not use global variable
index.js
Outdated
| // Filter the countries based on the input | ||
| const filteredCountries = allCountries.filter(country => | ||
| country.name.toLowerCase().includes(filterText) || // Match for name | ||
| country.capital.toLowerCase().includes(filterText) || // Match for capital |
There was a problem hiding this comment.
looks like you are iterating all over the values of the object to check if some of the values match.
If thats the case you can do something like this :
const filteredCountries = allCountries.filter(country =>
Object.values(country).some(value =>
value.toString().toLowerCase().includes(filterText)
)
);2 participants
Added js and modified the htmls.
Added 2 filtered funtions:
one by region and the other by user input
Fetched the data from the JSON and displayed it on the html.